🔍 Preloop Code Review

Last Updated: 2026-01-31 14:07:38Z
Reviewing Commit: 4b61888
Review Status: ❌ Changes Requested

📝 Summary

This PR migrates the UART protocol to a binary Avlos-style frame and reuses the generated endpoint dispatch. The overall direction looks good, but there are a couple of frame-validation issues that will break CRC checks and allow invalid payload lengths through.

✅ What Looks Good

• Clear transition to binary framing with CRC and endpoint dispatch reuse.
• UART low-level RX path is simplified and avoids ASCII protocol mixing.

⚠️ Issues Found

🟠 High Priority

• [Quality] CRC calculation excludes the last payload byte (CRC length uses len instead of len + 1) - firmware/src/uart/uart_interface.c:101
• [Quality] Payload length is not validated against the 8-byte buffer; oversized payloads pass uninitialized data to endpoints - firmware/src/uart/uart_interface.c:115

🟡 Medium Priority

• [Testing] No UART/Avlos frame parsing tests added for the new binary protocol (CRC, length bounds, read/write paths).

🟢 Low Priority

• [Quality] Response length is not bounded against UART_BYTE_LIMIT, risking TX buffer overflow if a callback returns an unexpected length - firmware/src/uart/uart_interface.c:58

✅ Resolved Issues

None yet.

📄 Documentation Impact

• README/docs: UART protocol description and framing details should be updated to match the new Avlos binary format.

Progress: 0 of 4 issues addressed

This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines.

🔍 Preloop Code Review

Last Updated: 2026-01-31 14:08:41 UTC
Reviewing Commit: 4b61888
Review Status: ❌ Changes Requested

📝 Summary

This PR refactors the UART path to a binary Avlos-style framing with CRC and ISR-driven TX/RX, which is a solid direction. However, CRC verification and payload-length validation issues will currently reject valid frames or accept malformed payloads, and the protocol change lacks test and documentation updates.

✅ What Looks Good

• Cleaned up UART initialization to the single in-use peripheral and IO mapping.
• CRC calculation via the hardware peripheral is efficient and consistent with embedded constraints.
• ISR TX/RX flow is simpler and easier to reason about than the prior ASCII parsing.

⚠️ Issues Found

🔴 Critical Issues

[Must be fixed before merge]

• None.

🟠 High Priority

[Should be addressed]

• [Quality] CRC verification uses the wrong length (drops valid frames) - firmware/src/uart/uart_interface.c:101

🟡 Medium Priority

[Suggestions for improvement]

• [Quality] Payload length is not validated before calling endpoints; uninitialized data can be used for writes - firmware/src/uart/uart_interface.c:117
• [Quality] Length byte only checked against buffer size, not protocol max/min - firmware/src/uart/uart_lowlevel.c:92
• [Testing] No UART protocol tests added for CRC, malformed frames, or read/write paths
• [Documentation] UART protocol change not reflected in user-facing docs

🟢 Low Priority

[Minor improvements, optional]

• None.

✅ Resolved Issues

None yet.

📄 Documentation Impact

• docs/interfaces/uart.rst: Update framing description to match binary Avlos + CRC format
• docs/protocol/overview.rst (or equivalent UART section): Note CRC requirement and payload limits

Progress: 0 of 5 issues addressed

This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines.

🔍 Preloop Code Review

Last Updated: 2026-01-31 14:31:32Z
Reviewing Commit: b8b4b65
Review Status: ❌ Changes Requested

📝 Summary

This PR migrates UART to the Avlos-style binary framing and reuses the Avlos endpoint dispatch. CRC length is corrected, but a duplicate buffer block currently breaks compilation and there are still guard/test/doc gaps to address.

✅ What Looks Good

• Clear binary framing with CRC and endpoint dispatch reuse.
• ISR-driven RX/TX path is simplified compared to the old ASCII protocol.

⚠️ Issues Found

🟠 High Priority

• [Quality] Duplicate buffer/data_len declarations in UART_process_message cause a compile error and bypass the validated buffer - firmware/src/uart/uart_interface.c:119

🟡 Medium Priority

• [Testing] No UART/Avlos frame parsing tests added (CRC, length bounds, read/write paths).
• [Documentation] UART protocol docs still describe the dot protocol and need the new binary Avlos framing.

🟢 Low Priority

• [Quality] Response length is not bounded against UART_BYTE_LIMIT, risking TX buffer overflow if a callback returns an unexpected length - firmware/src/uart/uart_interface.c:58

✅ Resolved Issues

• CRC verification length was off by one (len vs len + 1). - Fixed in b8b4b65
• Payload length validation was missing before endpoint dispatch. - Fixed in b8b4b65
• RX length byte needed protocol-bound validation at dispatch time. - Fixed in b8b4b65

📄 Documentation Impact

• docs/interfaces/interfaces.rst: Update UART protocol description to the binary Avlos framing (STX/LEN/CRC) and payload limits.

Progress: 3 of 7 issues addressed

This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines.

🔍 Preloop Code Review

Last Updated: 2026-01-31 14:36:27Z
Reviewing Commit: b8b4b65
Review Status: 💬 Changes Suggested

📝 Summary

This PR completes the shift to an Avlos-style binary UART frame and reuses the generated endpoint dispatch. The core parsing/CRC fixes look good, but test coverage and documentation updates are still missing, and there’s still a small TX-length guard to add.

✅ What Looks Good

• Clean binary framing with CRC and Avlos endpoint reuse.
• RX/TX ISR flow is simpler and more deterministic than the old ASCII parsing.

⚠️ Issues Found

🟡 Medium Priority

[Suggestions for improvement]

• [Testing] No UART/Avlos frame parsing tests added (CRC, length bounds, read/write paths)
• [Documentation] UART protocol docs still describe the dot protocol and need the binary Avlos framing - docs/interfaces/interfaces.rst:27

🟢 Low Priority

[Minor improvements, optional]

• [Quality] Response length is not bounded against UART_BYTE_LIMIT, risking TX buffer overflow if a callback returns an unexpected length - firmware/src/uart/uart_interface.c:58

✅ Resolved Issues

• CRC verification length was off by one (len vs len + 1). - Fixed in b8b4b65
• Payload length validation was missing before endpoint dispatch. - Fixed in b8b4b65
• RX length byte needed protocol-bound validation at dispatch time. - Fixed in b8b4b65
• Duplicate buffer/data_len declarations caused a compile error. - Fixed in b8b4b65

📄 Documentation Impact

• docs/interfaces/interfaces.rst: Replace dot-protocol description with the Avlos binary framing (STX/LEN/CRC) and payload limits.

Progress: 4 of 7 issues addressed

This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines.

Following extensive testing with client side Studio this presented several bugs that are hard to diagnose. Closing for now.